Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add --force command line option to run and deploy sub-commands #1568

Merged
merged 1 commit into from
Apr 18, 2019
Merged

Add --force command line option to run and deploy sub-commands #1568

merged 1 commit into from
Apr 18, 2019

Conversation

corneliusweig
Copy link
Contributor

@corneliusweig corneliusweig commented Jan 30, 2019 •
edited
Loading

As described in #1484, skaffold deploy should not force deployments, if this causes downtimes. This PR adds a new flag --force to the following commands with differing default values:

command force default hidden
dev true true
run false false
deploy false false

The flag is hidden always true for the dev command, because it would do more harm than good if somebody would try to disable forced deployments.

Fixes #1484

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Jan 30, 2019
@codecov-io
Copy link

codecov-io commented Jan 30, 2019 •
edited
Loading

Codecov Report

Merging #1568 into master will decrease coverage by 2.95%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1568      +/-   ##
==========================================
- Coverage   52.21%   49.25%   -2.96%     
==========================================
  Files         180      166      -14     
  Lines        7956     7297     -659     
==========================================
- Hits         4154     3594     -560     
+ Misses       3413     3357      -56     
+ Partials      389      346      -43
Impacted Files Coverage Δ
pkg/skaffold/config/options.go 88.23% <ø> (-1.24%) ⬇️
cmd/skaffold/app/cmd/config.go 100% <100%> (ø) ⬆️
pkg/skaffold/deploy/helm.go 62.39% <100%> (+0.47%) ⬆️
cmd/skaffold/app/cmd/fix.go 70% <100%> (ø) ⬆️
cmd/skaffold/app/cmd/version.go 46.15% <100%> (ø) ⬆️
pkg/skaffold/deploy/kubectl/cli.go 95.55% <100%> (+0.31%) ⬆️
cmd/skaffold/app/cmd/completion.go 12.5% <100%> (ø) ⬆️
cmd/skaffold/app/cmd/init.go 54.54% <100%> (ø) ⬆️
cmd/skaffold/app/cmd/cmd.go 71.76% <100%> (-2.43%) ⬇️
pkg/skaffold/deploy/kustomize.go 77.9% <100%> (+0.25%) ⬆️
... and 78 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26ae49f...b566986. Read the comment docs.

@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 30, 2019
@corneliusweig corneliusweig force-pushed the force-deploy branch 3 times, most recently from 71f4edc to 2e61c6a Compare January 31, 2019 01:05
@corneliusweig
Copy link
Contributor Author

@balopat @priyawadhwa So I just found that #1499 has some overlap with this one. There, a new deployer config option is added to the skaffold.yaml, instead of a command line flag. I think both approaches are valid (although I slightly prefer #1499 over mine here). I think a second opinion is required, because one way should be enough :)
Anyway, if you think that #1499 is better, it requires some more work to solve #1484 as well.

@priyawadhwa
Copy link
Contributor

Hey @corneliusweig, I think the --force flag implemented in #1499 is for helm specifically, and this PR is for kubectl (correct?).

I think you can continue with this PR, and the additional helm flags will be implemented as described here

@corneliusweig
Copy link
Contributor Author

Hi @priyawadhwa , assuming there will be no dedicated --force flag but a general helm flags list, I think you are right and we should proceed with this PR. Thanks for sorting that out :)

@corneliusweig corneliusweig force-pushed the force-deploy branch 4 times, most recently from ce6f1d9 to 684dd1a Compare February 7, 2019 22:44
@balopat balopat added kokoro:run runs the kokoro jobs on a PR and removed needs-rebase labels Feb 12, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Feb 12, 2019
balopat
balopat previously requested changes Feb 14, 2019
View reviewed changes
cmd/skaffold/app/cmd/cmd.go Outdated Show resolved Hide resolved
@corneliusweig corneliusweig force-pushed the force-deploy branch 3 times, most recently from b6415bc to 70c2006 Compare February 22, 2019 10:39
@corneliusweig corneliusweig force-pushed the force-deploy branch 2 times, most recently from ea4f112 to 9ebe005 Compare March 3, 2019 00:32
@corneliusweig
Copy link
Contributor Author

Does that PR require a special notice in the release notes, because it changes the behavior of skaffold run? What used to be skaffold run will then be skaffold run --force.

@corneliusweig corneliusweig force-pushed the force-deploy branch 4 times, most recently from 9fafaab to f4bd109 Compare March 14, 2019 22:03
@corneliusweig corneliusweig mentioned this pull request Mar 16, 2019
Closed
@corneliusweig corneliusweig force-pushed the force-deploy branch 5 times, most recently from c06482b to dbe78ca Compare March 24, 2019 22:14
@nkubala
Copy link
Contributor

nkubala commented Mar 26, 2019

I think this is ready to go, Kokoro got hung up so I restarted the job. once this is all green I'll merge

nkubala
nkubala suggested changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, would you mind expanding a tiny bit on the CLI documentation to detail the default values for a given command? (e.g. defaults to "false" for run, "true" for dev, etc.)

@corneliusweig
Copy link
Contributor Author

@nkubala I don't fully understand what you mean. The flag is only valid for run and deploy, but not for dev. Can you explain a bit more where you want the docs to be updated? The usage string does not seem to be the right place to me.

@nkubala
Copy link
Contributor

nkubala commented Apr 1, 2019

@corneliusweig I think it's fine to just add a tiny bit to the usage string. something like default: true for dev, false for run. WDYT?

side note: @balopat and I both wish there was a better place for this sort of extended documentation to be added, but we couldn't really come up with a better solution :(

@corneliusweig corneliusweig force-pushed the force-deploy branch 2 times, most recently from 25a2e67 to 70556f2 Compare April 2, 2019 00:18
@corneliusweig
Copy link
Contributor Author

corneliusweig commented Apr 2, 2019 •
edited
Loading

@nkubala Ok, I finally got your point. For some reason, I believed that cobra always prints info about defaults for a flag.
So I added the default, I hope it's ok like that. However, I would not want to mention the dev command in the default value, because the flag does not even exist there (its always implied). Saying default: true for dev would be confusing IMHO.

@corneliusweig
Copy link
Contributor Author

caveat: This had a lot of conflicts after merging of RunContext.

@corneliusweig
Add new flag '--force' to run/deploy commands
b566986 
This flag controls, if resources may be re-created, if patching is not possible.
Summary of changes:
- Add force-deploy option to kubectl and kustomize deployer
- Add force-deploy option to helm deployer
- Reduce code duplication in helm_test.go

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@corneliusweig
Copy link
Contributor Author

@nkubala Could you take another look? This PR has been open for ages now. I also wouldn't mind if you re-word the help text of the new flag ;)

@nkubala nkubala merged commit 6bd7080 into GoogleContainerTools:master Apr 18, 2019
@corneliusweig corneliusweig deleted the force-deploy branch April 18, 2019 20:54
@nkubala nkubala mentioned this pull request Jul 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@balopat balopat balopat left review comments

@nkubala nkubala nkubala approved these changes

@dgageot dgageot Awaiting requested review from dgageot

@priyawadhwa priyawadhwa Awaiting requested review from priyawadhwa

@r2d4 r2d4 Awaiting requested review from r2d4

Assignees

@balopat balopat

Labels
cla: yes needs-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

skaffold deploy should not automatically re-create resources
8 participants
@corneliusweig @codecov-io @priyawadhwa @balopat @nkubala @dgageot @googlebot @kokoro-team